fix(MonkeyPatch): register undo entry only after a successful mutation (delattr/setitem/delitem) (#14909) - #14910
Conversation
psf-chronographer
Bot
added
the
bot:chronographer:provided
|
Just for context, I had already been working on this before opening #14909 and had a tested fix with regression tests ready locally. I was waiting for assignment/maintainer feedback before opening my PR, as I mentioned in the issue. Happy to follow the maintainers preference on how to proceed with the overlapping work. |
d10c3b6 to
020f8f1
Compare
…n (delattr/setitem/delitem) (pytest-dev#14909) Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
020f8f1 to
a88e91b
Compare
NikolayAir
left a comment
NikolayAir
left a comment
There was a problem hiding this comment.
I verified locally that the three regression tests for failed delattr, setitem, and delitem operations fail against the PR base implementation because a failed operation still leaves an undo entry behind even though no change was made. That entry can later cause undo() or automatic test cleanup to fail. All three tests pass on the PR head.
testing/test_monkeypatch.py also passes locally (39 passed, 1 skipped). The implementation now records each undo entry only after the corresponding operation succeeds, matching the existing setattr pattern. Based on these checks, the fix looks correct for the reported cases.
| monkeypatch.undo() | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("make_mapping", [dict]) |
There was a problem hiding this comment.
Optional: make_mapping is parametrized here, but the test doesn't use it. Instead, inner is created directly as a dict. Since dict is the only value listed here, would it be clearer to remove the parametrization?
If additional mapping types are intended, using make_mapping(...) to create inner would make that intent explicit.
There was a problem hiding this comment.
Good catch, thanks. I pushed 2802b66 to remove the unused parametrization and keep the test as the single concrete dict case it actually exercises.
Validation: . .venv/bin/activate && python -m pytest testing/test_monkeypatch.py -q -> 39 passed, 1 skipped.
There was a problem hiding this comment.
Thanks for the quick update. Removing the unused parametrization makes the test clearer.
3 participants
What
Fixes #14909.
MonkeyPatch.delattr,MonkeyPatch.setitemandMonkeyPatch.delitemregistered their undo entry before attempting the underlying mutation. When the mutation raised (e.g.delattron a slotted attribute, orsetitem/delitemon a read-onlyMappingProxyType), the target was left unchanged but a stale undo entry remained on the stack. Later,undo()(called automatically at fixture teardown) tried to revert a change that never happened, producing a spurious teardown error.How
Applied the same ordering already used by
MonkeyPatch.setattrafter #14209:The success-case behavior of each method is unchanged.
Tests
Added regression tests mirroring the reproduction in the issue:
test_failed_delattr—delattron a slotted attribute,test_failed_setitem—setitemon aMappingProxyType,test_failed_delitem—delitemon aMappingProxyType.Each asserts the target is unchanged after the expected exception and that
undo()does not raise (i.e. no stale undo entry is left behind). A changelog fragment was added.